Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

AVRO-4115: [PHP] Fix long encoding #3303

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

AVRO-4115: [PHP] Fix long encoding #3303

wants to merge 1 commit into from

Conversation

jonhkr
Copy link

@jonhkr jonhkr commented Jan 27, 2025

What is the purpose of the change

This PR fixes PHP long encoding on 64bit platforms as the current implementation does not support longs bigger than 1<<61.

Verifying this change

I've added new tests to cover more scenarios and uncommented existing tests.

https://issues.apache.org/jira/browse/AVRO-4115

@jonhkr
Copy link
Author

jonhkr commented Jan 27, 2025

@thiagorb @MichelHartmann can you guys take a look at this please?

@jonhkr jonhkr changed the title Fix long encoding AVRO-4115: [PHP] Fix long encoding Jan 28, 2025
@thiagorb
Copy link
Contributor

Looks good to me!

lang/php/lib/AvroVarint.php Outdated Show resolved Hide resolved
lang/php/lib/AvroVarint.php Outdated Show resolved Hide resolved
@jonhkr
Copy link
Author

jonhkr commented Feb 10, 2025

@martin-g can you take a look again please? I decided to keep the original methods instead of creating a new file, the diff looks better now.

@martin-g
Copy link
Member

@adamquaile @valvadis @jjatria @skystebnicki You might be interested to review this change

@jonhkr jonhkr force-pushed the main branch 2 times, most recently from aea815f to 4ca2932 Compare February 10, 2025 15:53
@jonhkr
Copy link
Author

jonhkr commented Feb 10, 2025

@martin-g there was a small typo in the Decoder which caused the tests to fail, should be fixed now. Can you allow the CI to run please?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants